Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use centered modal for errors in the VBA flow in NewDot #4483

Merged
merged 5 commits into from
Aug 10, 2021

Conversation

chiragsalian
Copy link
Contributor

@chiragsalian chiragsalian commented Aug 7, 2021

Details

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/171666#

Tests / QA

  1. Click on the green + icon on the botton left and select new workspace if you dont already have one.
  2. Click Get Started -> Connect manually
  3. Use this SO and manually enter details.
  4. When you see the company information screen enter incorrect values and click save and confirm.
  5. Confirm you see the alert modal,
    image

QA Steps

  1. Same as above.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

image

Mobile Web

Desktop

iOS

image

Android

@chiragsalian chiragsalian self-assigned this Aug 7, 2021
@chiragsalian chiragsalian marked this pull request as ready for review August 7, 2021 00:49
@chiragsalian chiragsalian requested a review from a team as a code owner August 7, 2021 00:49
@MelvinBot MelvinBot requested review from marcaaron and removed request for a team August 7, 2021 00:49
Copy link
Contributor

@marcaaron marcaaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good but unsure if they are conflicting. Maybe @MitchExpensify @jasperhuangg knows for sure.

@@ -66,47 +67,38 @@ class CompanyStep extends React.Component {
*/
validate() {
if (!this.state.password.trim()) {
Growl.error(this.props.translate('common.passwordCannotBeBlank'));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a quick question since I'm not really up to speed on this...

In this PR, we are replacing all these calls with red outlines and inline messages -> #4431

Do we need both a confirm modal and the inline messages?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I gathered from the solution mentioned in the issue.

Specific fields causing errors will be highlighted thanks to this change so we can use a generic message in the modal for all cases:
"Oops something went wrong! Please double check any highlighted fields and try again."

We can still wait for @MitchExpensify to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes we're replacing all growls with centered modals as well as adding red field outlines.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@chiragsalian
Copy link
Contributor Author

Updated to address conflicts.

@chiragsalian
Copy link
Contributor Author

it took 43mins to pass,
image
crazy. Anyway looks like we're all clear to merge here.

@marcaaron
Copy link
Contributor

it took 43mins to pass,

Yes, it takes a long time to build the app for testing, but a necessary evil.

@marcaaron marcaaron merged commit 808a307 into main Aug 10, 2021
@marcaaron marcaaron deleted the chirag-center-prompt branch August 10, 2021 17:58
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@chiragsalian chiragsalian mentioned this pull request Aug 11, 2021
5 tasks
@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @marcaaron in version: 1.0.83-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @isagoico in version: 1.0.85-9 🚀

platform result
🤖 android 🤖 failure ❌
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants